Skip to content

Point workspaceFolder to /workspaces/training/ #500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 17, 2025

Conversation

maxulysse
Copy link
Collaborator

@maxulysse maxulysse commented Feb 13, 2025

Continuing a discussion with @robsyme
We played with GitHub Codespaces and we noticed that it was errorring and not expending the value from ${localWorkspaceFolder}.

2025-2-13_15-23-23

I propose to point directly to /workspaces/training/
Screenshot from 2025-02-13 16-04-18

EDIT: reflect what actually happened in that PR

Copy link

netlify bot commented Feb 13, 2025

Deploy Preview for nextflow-training ready!

Name Link
🔨 Latest commit af5a459
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-training/deploys/67b35a777bf30400086e59aa
😎 Deploy Preview https://deploy-preview-500--nextflow-training.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@robsyme
Copy link
Contributor

robsyme commented Feb 13, 2025

The only issue here is that this hard-codes the hello-nextflow series as the only workspace, and wouldn't work for other training materials. I think we need to find a way to specify the workspace.

@maxulysse
Copy link
Collaborator Author

The only issue here is that this hard-codes the hello-nextflow series as the only workspace, and wouldn't work for other training materials. I think we need to find a way to specify the workspace.

I think we need to see it in a different way.
We have a quick intro to bash and CLI, let's add a quick intro to VSCode and explain how to change workspace, but by default you're in this small one

@pinin4fjords
Copy link
Collaborator

It's honestly not that hard to jump into a folder and exclude the rest, @adamrtalbot had people doing that in the last training I was on with him, even for folders within 'hello'. In the absence of a linkable way of specifying a workspace, it's better people start at the top and we show them how to do that (IMO).

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging now and asking forgiveness later 🤠

@ewels ewels merged commit 6fc3587 into nextflow-io:master Feb 17, 2025
7 checks passed
@pinin4fjords
Copy link
Collaborator

image

@vdauwera
Copy link
Collaborator

Merging now and asking forgiveness later 🤠

Doh, I missed this. Two things:

  1. Please check with all stakeholders before changing a default, even if it's reverting a behavior to how it had been previously at some point.

  2. I'm pleased to see this isn't actually working as described, because in the meantime some of us discussed this and (in line with @pinin4fjords 's comment above) we agreed to keep the entrypoint to be the top level directory and just document that people have to cd into the correct directory.

Corollary: either this got reverted or what was put in doesn't work? I'd like to know which it is, to make sure we understand current behavior and nothing is going to changed from under our feet.

@ewels
Copy link
Member

ewels commented Feb 21, 2025

  1. This was a hard failure before this PR - GitHub Codespaces wouldn't load at all. It threw a big ugly error and then rebooted with the default settings and container. Hence it needed an urgent merge in order to unblock usage of Codespaces.
  2. The PR title is wrong. Maxime did check on slack and changed the PR to go to the root folder instead of nf-hello. So no change in behaviour. So it is working as intended.

So it didn't get reverted and it didn't not work - it was changed to not follow the initial title. If you check the files diff on the PR you'll see the file path doesn't include nf-hello.

@maxulysse maxulysse changed the title Point workspaceFolder to /workspaces/training/hello-nextflow/ Point workspaceFolder to /workspaces/training/ Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants